feat: make wallet_sendCalls more reliable
#7816
Conversation
jiexi
changed the title
wallet_sendCalls more reliable
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| address: Hex, | ||
| req: JsonRpcRequest, | ||
| { getAccounts }: { getAccounts: (req: JsonRpcRequest) => Promise<string[]> }, | ||
| { getPermittedAccountsForOrigin }: { getPermittedAccountsForOrigin: () => Promise<string[]> }, |
There was a problem hiding this comment.
Could we avoid breaking changes for all clients if we kept the current name, but just ensured the hooks check the origin?
There was a problem hiding this comment.
Yeah Jiexi and I went back and forth on this a bit, but I think for this case the name change is worth it. The bug this PR fixes happened precisely because getAccounts is ambiguous - it doesn't convey that these must be permitted accounts for the requesting origin. A future developer could reasonably implement getAccounts by returning all accounts or the selected account, not realizing this breaks the security model.
Since this is already a breaking change (the signature changed from (req) => Promise<string[]> to () => Promise<string[]>), we'd prefer to make the API self-documenting and reduce the risk of similar bugs.
| // Ensure that an "unauthorized" error is thrown if the requester | ||
| // does not have the `eth_accounts` permission. | ||
| const accounts = await getAccounts(req); | ||
| const accounts = await getPermittedAccountsForOrigin(); |
There was a problem hiding this comment.
How does the client access the origin if we don't provide anything to the hook?
There was a problem hiding this comment.
The hook gets the origin off the request object directly. The 7702 middleware follows the same pattern that I'm hoping to use here
|
|
||
| if (!from) { | ||
| throw providerErrors.unauthorized(); | ||
| } |
There was a problem hiding this comment.
Unneeded accounts lookup can break sendCalls
Medium Severity
processSendCalls now always awaits getPermittedAccountsForOrigin() even when params.from is provided. This introduces a new failure point and latency for requests that previously did not need any accounts lookup, so transient hook errors can now reject otherwise-valid wallet_sendCalls requests.
| // The first account returned by `getPermittedAccountsForOrigin` is the selected account for the origin | ||
| const [selectedAccount] = await getPermittedAccountsForOrigin(); | ||
| const from = paramFrom ?? selectedAccount; | ||
|
|
There was a problem hiding this comment.
Sender selection relies on account ordering
High Severity
When from is omitted, processSendCalls uses the first element returned by getPermittedAccountsForOrigin() as the selected account. The hook’s contract only says “permitted accounts”, so if ordering is not guaranteed, the chosen sender can be the wrong permitted account.
3 participants
Explanation
Improves reliability of wallet_sendCalls.
Changes
getAccountshook togetPermittedAccountsForOriginhook which takes no params and returns the permitted accounts for the origin of the requestReferences
Checklist
Note
Medium Risk
Changes the public hook surface from
getAccounts(req)togetPermittedAccountsForOrigin()and makesoriginmandatory forwallet_sendCalls/wallet_getCapabilities, which may require consumer updates. Also changeswallet_sendCallsdefaultfromselection and unauthorized error behavior when no permitted accounts exist.Overview
Improves
wallet_sendCallsreliability by switching account authorization to an origin-scopedgetPermittedAccountsForOrigin()hook and using its first returned account as the defaultfromwhen the request omitsfrom(throwingproviderErrors.unauthorized()if none are permitted).Updates
wallet_sendCalls,wallet_getCapabilities, andvalidateAndNormalizeKeyholderto use the new hook signature and requireoriginon requests, and adjusts/extends tests plus the package changelog accordingly.Written by Cursor Bugbot for commit f77bd57. This will update automatically on new commits. Configure here.